Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: turn streaming on by default #2028

Merged
merged 9 commits into from
Mar 27, 2025
Merged

feat: turn streaming on by default #2028

merged 9 commits into from
Mar 27, 2025

Conversation

astandrik
Copy link
Collaborator

@astandrik astandrik commented Mar 20, 2025

Closes #2024

Stand

CI Results

Test Status: ✅ PASSED

📊 Full Report

Total Passed Failed Flaky Skipped
286 285 0 0 1
Test Changes Summary ✨4 ⏭️1

✨ New Tests (4)

  1. Timeout input is invisible by default (tenant/queryEditor/querySettings.test.ts)
  2. Clicking timeout switch makes timeout input visible (tenant/queryEditor/querySettings.test.ts)
  3. Timeout switch is checked, disabled, and has hint when non-query mode is selected (tenant/queryEditor/querySettings.test.ts)
  4. When Query Streaming is off, timeout has label and input is visible by default (tenant/queryEditor/querySettings.test.ts)

⏭️ Skipped Tests (1)

  1. Streaming query shows some results and banner when stop button is clicked (tenant/queryEditor/queryEditor.test.ts)

Bundle Size: 🔺

Current: 83.24 MB | Main: 83.23 MB
Diff: +9.87 KB (0.01%)

⚠️ Bundle size increased. Please review.

ℹ️ CI Information
  • Test recordings for failed tests are available in the full report.
  • Bundle size is measured for the entire 'dist' directory.
  • 📊 indicates links to detailed reports.
  • 🔺 indicates increase, 🔽 decrease, and ✅ no change in bundle size.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request enables query streaming by default and refactors the UI for query timeout settings.

  • Introduces a new QuerySettingsTimeout component to handle timeout inputs.
  • Updates the QuerySettingsDialog to replace inline timeout control with the new component and changes the docs link from ExternalLink to Button.
  • Adjusts default streaming settings and the capability version check for streaming.

Reviewed Changes

Copilot reviewed 5 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/containers/Tenant/Query/QuerySettingsDialog/QuerySettingsTimeout.tsx New component to encapsulate timeout settings in the query dialog.
src/containers/Tenant/Query/QuerySettingsDialog/QuerySettingsDialog.tsx Integrates the new timeout component and updates the docs link.
src/utils/query.ts Updates the timeout field validation schema by removing the default value catch.
src/services/settings.ts Changes default query streaming setting to true.
src/store/reducers/capabilities/hooks.ts Updates the minimum version requirement for streaming availability.
Files not reviewed (4)
  • src/containers/Tenant/Query/QuerySettingsDialog/QuerySettingsDialog.scss: Language not supported
  • src/containers/Tenant/Query/QuerySettingsDialog/QuerySettingsTimeout.scss: Language not supported
  • src/containers/Tenant/Query/QuerySettingsDialog/i18n/en.json: Language not supported
  • src/containers/Tenant/Query/QuerySettingsDialog/i18n/ru.json: Language not supported
Comments suppressed due to low confidence (1)

src/utils/query.ts:332

  • Removing the default catch for the timeout value might lead to issues when processing empty or invalid inputs. If a default value is desired, consider handling it explicitly.
z.coerce.number().positive().optional()

@astandrik astandrik removed the request for review from artemmufazalov March 20, 2025 09:18
@astandrik astandrik closed this Mar 20, 2025
@astandrik astandrik reopened this Mar 20, 2025
@astandrik astandrik marked this pull request as draft March 20, 2025 09:34
@astandrik astandrik marked this pull request as ready for review March 25, 2025 13:04
content={QUERY_SETTINGS_FIELD_SETTINGS.timeout.title}
/>
{isDisabled ? (
<Popover
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


const isChecked = value !== null;

const queryStreamingLabel = isQueryStreamingEnabled ? (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe rename to timeoutLabel? And I would do it a separate component (not a demand, just suggestion).

className={b('switch')}
content={QUERY_SETTINGS_FIELD_SETTINGS.timeout.title}
/>
{isDisabled ? (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems && is enough?

@astandrik astandrik requested a review from Raubzeug March 26, 2025 13:28
@astandrik
Copy link
Collaborator Author

@Raubzeug fixed

@astandrik astandrik added this pull request to the merge queue Mar 27, 2025
Merged via the queue into main with commit bc5d59d Mar 27, 2025
7 checks passed
@astandrik astandrik deleted the astandrik.2024 branch March 27, 2025 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

turn streaming on by default
2 participants